Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

yaml/yml label file if/else guards #23717

Closed
wants to merge 2 commits into from
Closed

yaml/yml label file if/else guards #23717

wants to merge 2 commits into from

Conversation

eeyrjmr
Copy link
Contributor

@eeyrjmr eeyrjmr commented Mar 26, 2023

ensure the legacy label parser is not called if a yaml file is detected. fixes #23715

Signed-off-by: Jon Roadley-Battin jon.roadleybattin@gmail.com

data, err = options.Labels(name)
if err != nil {
return nil, ErrTemplateLoad{name, fmt.Errorf("GetRepoInitFile: %w", err)}
if filepath.Ext(name) == ". yaml" {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some questions:

  1. ". yaml": a space in it?
  2. Gitea has builtin Advanced.yaml, if a user puts a customized Advance.yml, then still the builtin Advanced.yaml is used?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Typo (will correct )
  2. Good question. I need to check but the built-in Advanced.yaml isn't in the custom options directory. It does raise the question that if some adds foo.yaml and foo.yml ... What should be used

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Mar 26, 2023
@zeripath
Copy link
Contributor

zeripath commented Mar 26, 2023

I don't think this is right.

Is the intention here that if name has a suffix .yaml then parseYamlFormat will be run on that file? Because that is not what this is doing...

See #23719

@eeyrjmr
Copy link
Contributor Author

eeyrjmr commented Mar 26, 2023

I don't think this is right.

Is the intention here that if name has a suffix .yaml then parseYamlFormat will be run on that file? Because that is not what this is doing...

See #23719

The intent was if the suffix was yaml then execute the parser but I don't think your proposed solution actually solves the problem.
It might be best to close this MR and continue the discussion in your's

zeripath added a commit to zeripath/gitea that referenced this pull request Mar 27, 2023
…o options

The previous code did not remove the .yaml/.yml extension from label
files within the customPath. This would lead to duplicate lists of
labels.

Fix go-gitea#23715
Close go-gitea#23717

Signed-off-by: Andrew Thornton <art27@cantab.net>
@eeyrjmr eeyrjmr closed this Mar 27, 2023
lunny pushed a commit that referenced this pull request Apr 10, 2023
Fix #23715

Other related PRs:

* #23717
* #23716
* #23719

This PR is different from others, it tries to resolve the problem fundamentally (and brings more benefits)

Although it looks like some more lines are added, actually many new lines are for tests.

----

Before, the code was just "guessing" the file type and try to parse them.

<details>

![image](https://user-images.githubusercontent.com/2114189/228002245-57d58e27-1078-4da9-bf42-5bc0b264c6ce.png)

</details>

This PR:

* Always remember the original option file names, and always use correct parser for them.

* Another benefit is that we can sort the Label Templates now (before there was a map, its key order is undefined)

![image](https://user-images.githubusercontent.com/2114189/228002432-931b9f18-3908-484b-a36b-04760c9ad132.png)
wxiaoguang added a commit to wxiaoguang/gitea that referenced this pull request Apr 12, 2023
…23749)

Fix go-gitea#23715

Other related PRs:

* go-gitea#23717
* go-gitea#23716
* go-gitea#23719

This PR is different from others, it tries to resolve the problem fundamentally (and brings more benefits)

Although it looks like some more lines are added, actually many new lines are for tests.

----

Before, the code was just "guessing" the file type and try to parse them.

<details>

![image](https://user-images.githubusercontent.com/2114189/228002245-57d58e27-1078-4da9-bf42-5bc0b264c6ce.png)

</details>

This PR:

* Always remember the original option file names, and always use correct parser for them.

* Another benefit is that we can sort the Label Templates now (before there was a map, its key order is undefined)

![image](https://user-images.githubusercontent.com/2114189/228002432-931b9f18-3908-484b-a36b-04760c9ad132.png)
# Conflicts:
#	modules/label/parser.go
#	routers/web/org/setting.go
#	routers/web/repo/issue_label.go
#	templates/repo/create.tmpl
#	templates/repo/issue/labels/label_load_template.tmpl
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

custom label option appears twice
4 participants